Codecov Report

Merging #1064 (c07d621) into master (c89b767) will increase coverage by 0.42%.
The diff coverage is 82.75%.

@@            Coverage Diff             @@
##           master    #1064      +/-   ##
==========================================
+ Coverage   71.72%   72.14%   +0.42%     
==========================================
  Files         381      397      +16     
  Lines       11918    12232     +314     
  Branches     2600     2648      +48     
==========================================
+ Hits         8548     8825     +277     
- Misses       3339     3368      +29     
- Partials       31       39       +8     

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c89b767...c07d621. Read the comment docs.

Thanks @wayneseymour for your PR!

• The api/charts.api.md file can be regenerated with yarn build:ts && yarn api:extract and the output file (that gets output a different location) can becp tmp/charts.api.md api/charts.api.md-ed in place of api/charts.api.md and checked eg. via diffing if the changes are as you expect
• The source change looks good, wondering @markov00 what our policy is for approving/merging PRs that break the API without a linked requirement by the dependency, Kibana—I guess it's best to batch such changes to minimize the # of BREAKING. But batching means, longer wait till it hits master, with inevitable merge conflicts, as a maintenance PR impacts many lines. Future additions of ChartTypes also won't be covered (TS checks should catch this). Ie. there's more work than usual on the part of the PR maker and whoever works on future merges
• It's good to run tests locally too, including, in such cases when many files are changed, the image tests, glad to help setting it up; besides firing up storybook and testing a variety of charts manually
• Tre' a note for merge time, elastic-charts has commit message content conventions for breaking changes, see here

Tre' one note is that sometimes we're assisting Kibana users in such API changes, eg. via small PRs there by the chart PR maker 😉 That's also a reason for, I think, merging a bunch of API breaking changes in one fell swoop, Marco what do you think, with 7.12 around the corner?

Thank you Tre'!
This should definitely include the BREAKING CHANGE footer
We can merge this and update 7.13 branch, it will not be merged on 7.12 as we can't introduce breaking changes after FF

I think it's fine to include multiple changes here, some enums are not currently used much outside the library

Thanks @wayneseymour this issue is very near and dear to my ❤️

@monfera I think this is just a nice to have that will eventually get into 7.13 so I don't think we need to be too critical of batching other PRs nor waiting to get others merged before this as we could just backport any critical changes to 7.12 in the few remaining days. Also we end up having to merge the PRs so one of us should be sure to note the BREAKING CHANGE.

Thanks @wayneseymour for your PR!

• The api/charts.api.md file can be regenerated with yarn build:ts && yarn api:extract and the output file (that gets output a different location) can becp tmp/charts.api.md api/charts.api.md-ed in place of api/charts.api.md and checked eg. via diffing if the changes are as you expect
• The source change looks good, wondering @markov00 what our policy is for approving/merging PRs that break the API without a linked requirement by the dependency, Kibana—I guess it's best to batch such changes to minimize the # of BREAKING. But batching means, longer wait till it hits master, with inevitable merge conflicts, as a maintenance PR impacts many lines. Future additions of ChartTypes also won't be covered (TS checks should catch this). Ie. there's more work than usual on the part of the PR maker and whoever works on future merges
• It's good to run tests locally too, including, in such cases when many files are changed, the image tests, glad to help setting it up; besides firing up storybook and testing a variety of charts manually
• Tre' a note for merge time, elastic-charts has commit message content conventions for breaking changes, see here

Tre' one note is that sometimes we're assisting Kibana users in such API changes, eg. via small PRs there by the chart PR maker 😉 That's also a reason for, I think, merging a bunch of API breaking changes in one fell swoop, Marco what do you think, with 7.12 around the corner?

Awesome, I'll get on this next week! "Shut it down day" tomorrow in the US :)

Thank you Tre'!
This should definitely include the BREAKING CHANGE footer
We can merge this and update 7.13 branch, it will not be merged on 7.12 as we can't introduce breaking changes after FF

I think it's fine to include multiple changes here, some enums are not currently used much outside the library

Perfect, I'll fixup asap! Thanks for reviewing!

@monfera @nickofthyme Gents, I'm not sure how to proceed here.
The issue is with symbol shadowing. In this case, the object and the type would have the same name if I refactored away the plurality.
Ideas?

@wayneseymour that's fine, please see eg. LegendStrategy or other similar symbols that get dual-bound (one value, one type) as part of our current approach of working around (or rather, avoid) TypeScript enums

export type LegendStrategy = Values<typeof LegendStrategy>;

Locally, I've run these build scripts and they passed:

• yarn build:ts && yarn api:extract
• yarn lint:fix
• yarn test

LGTM, scanned all the changes online and looks great!
Thanks for taking the time for that.
Just remember to add a breaking change line in the merge commit like the following:

your PR description

BREAKING CHANGE: `AnnotationDomainTypes`, `AnnotationType` and all the others (please list them) are renamed to ....

Indeed I will! Thanks @markov00

🎉 This PR is included in version 28.0.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Gents, I'm not sure how to proceed here.
The issue is with symbol shadowing. In this case, the object and the type would have the same name if I refactored away the plurality.
Ideas?

Hey @wayneseymour I was sick last week so I didn't see this till now and wanted to clear this up in case you ran into this in the future. I'm thinking this might be an issue with the linter because in typescript when exporting a type or value you create a separate entity for each, so the naming will not clash when doing something like...

export const SeriesType = Object.freeze({
  Area: 'area' as const,
  Bar: 'bar' as const,
  Line: 'line' as const,
  Bubble: 'bubble' as const,
});
export type SeriesType = $Values<typeof SeriesType>;

Typescript calls this Declaration Merging whereby the type and value (and namespaces) are merged into one as SeriesType wherever it is imported. In this case, we call this a const enum as it is not a typescript enum, see elastic/kibana#62957 (comment) for a deeper explanation of why.